Align documentation about HTTPRouteTimeouts.BackendRequest timeout scope #3462
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
do-not-merge/hold
|
Hi @rtribotte. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test This seems reasonable, but I'll need to have a think about it in case it counts as a breaking change. |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
needs-rebase
There was a problem hiding this comment.
Thanks @rtribotte! The Traefik related changes to the GEP look great, thanks for the corrections. Want to get more feedback on the changes to the timeout definition.
/hold
apis/v1/httproute_types.go
Outdated
| @@ -347,7 +347,7 @@ type HTTPRouteTimeouts struct { | |||
|
|
|||
| // BackendRequest specifies a timeout for an individual request from the gateway | |||
| // to a backend. This covers the time from when the request first starts being | |||
| // sent from the gateway to when the full response has been received from the backend. | |||
| // sent from the gateway to when the response headers have been received from the backend. | |||
There was a problem hiding this comment.
This seems like a significant change, would appreciate some additional reviews on this.
There was a problem hiding this comment.
IIUC this breaks Envoy implementations?
There was a problem hiding this comment.
Yes, in Envoy implementations, there's no timeout for the end of the request, only the end of the headers. So this would not be possible to fulfill in any Envoy-based implementation.
From the update to the Traefik timeouts, I can see that Traefik doesn't support an end-of-headers timeout, which puts us in a tricky position (and is one of the reasons why this GEP took so long to get done).
Edit: I was looking at the request timeouts, not the response ones, sigh.
I think in this case, the godoc is the correct behavior, and we should be changing the diagram to match the godoc.
(In general, for Gateway API, the godoc is the canonical location for specification things, so if there's a mistake, we fix other things first, as the godoc is part of the stable version).
This field is now Standard and so this is definitely a breaking change that we can't do without an API revision.
Sorry @rtribotte this is exactly the sort of thing I was worried about when I said I needed to do a closer read.
There was a problem hiding this comment.
Isn't it the opposite? Envoy can do "end of response" not "end of response headers"
There was a problem hiding this comment.
🤦 Looking at the wrong timeout section. Let me edit my comment.
There was a problem hiding this comment.
Thanks and sorry for the delay here.
There was a problem hiding this comment.
@youngnick I have updated the PR, nonetheless, I have a question regarding the timeouts.request end time, the go doc says:
Request specifies the maximum duration for a gateway to respond to an HTTP request.
but also
This timeout is intended to cover as close to the whole request-response
According to this, I think the timeouts.request end time is not well placed in diagram, WDYT?
There was a problem hiding this comment.
That's why it says "as close as" - from my reading of the timeouts available across implementations, there's not enough commonality to be able to say "this timeout covers exactly from here to here and only those things" - whatever we pick, some implementation can't do it.
There was a problem hiding this comment.
@youngnick Ok, so if it is better to remain vague to match most implementations, could I reflect this tolerance margin on the diagram, as for the timeouts.request start time (min and max)?
It makes me wonder why the tolerance margin, regarding the exact covering a timeout has, has not been introduced when defining the timeouts.backendRequest timeout as well.
Now that the timeouts.backendRequest covering is clarified and aligned in the documentation, it suits Envoy implementation, but not Traefik.
Would you be inclined to introduce a tolerance margin that suits the Traefik implementation?
There was a problem hiding this comment.
Having a tolerance margin as a field? I need to think about that for a while, sorry.
k8s-ci-robot
requested review from
mlavacca,
youngnick,
howardjohn and
arkodg
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: kate-osborn. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
rtribotte
force-pushed
the
fix-backendtimeout-doc
branch
from
14f3878 to
65d5bf4
Compare
k8s-ci-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rtribotte The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
One small thing while we're fixing the last sequence diagram, but I think this looks as good as we can do given the multiple-datapath limitations. |
k8s-ci-robot
added
size/S
What type of PR is this?
/kind documentation
What this PR does / why we need it:
This PR aligns the
HTTPRouteTimeouts.BackendRequesttimeout documentation.It is stated in the GEP 1742 sequence diagram illustrating the timeouts boundaries that the
HTTPRouteTimeouts.BackendRequesttimeout runs until the responses headers are received:gateway-api/geps/gep-1742/index.md
Lines 358 to 360 in f735045
The documentation for the CRD was mentioning a timeout running until the end of the response, which is not consistent and this PR aims to align expectations.
On the side, in the GEP 1742, the Traefik mermaid sequence diagram is adjusted to reflect more accurately the actual supported timeouts.
Which issue(s) this PR fixes:
Does this PR introduce a user-facing change?: